-
Notifications
You must be signed in to change notification settings - Fork 168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
7455: Add support for jolokia JMX service connection #548
Conversation
👋 Welcome back skarsaune! A progress list of the required criteria for merging this PR into |
@skarsaune this pull request can not be integrated into git checkout jolokia-support-only
git fetch https://git.openjdk.org/jmc.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
Webrevs
|
d477ccf
to
ffe7eab
Compare
@skarsaune Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave this a quick read through, the builds aren't happy because a couple of the imports need to be updated in ServerConnectionDescriptor
and JolokiaTest
; both need IConnectionDescriptor
and IServerDescriptor
to be updated to come from rjmx.common
. After these changes it looks like everything builds and tests pass.
I've commented on a handful more nits I found, but haven't actually tested the functionality yet so I can't comment on anything else.
Another PR where the check license script isn't happy though, I'll have to a take a look at why it's trying to check all these different files that weren't touched by your commit.
...rg.openjdk.jmc.jolokia/src/main/java/org/openjdk/jmc/jolokia/ServerConnectionDescriptor.java
Outdated
Show resolved
Hide resolved
application/tests/org.openjdk.jmc.jolokia.test/META-INF/MANIFEST.MF
Outdated
Show resolved
Hide resolved
application/tests/org.openjdk.jmc.jolokia.test/META-INF/MANIFEST.MF
Outdated
Show resolved
Hide resolved
...on/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java
Outdated
Show resolved
Hide resolved
...on/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java
Outdated
Show resolved
Hide resolved
Great, that is very helpful. Thanks for the quick feedback. |
Update: Something does not seem to work after updating to master. I will debug a bit, but do not have the time the next couple of days |
Ok, so at least in JMC 7.1.0 and 8.3.0 this use to work by the fact that the plugin uses the extension point for JMX protocol: For instance when trying to connect to a JMC console from the JVM browser I get this:
This appears to use Javas native JMXConnectorFactory directly . This may fail for a number of reasons:
Anyone know more about the setup of connections or how this may have changed recently. |
Hmm, or is |
Update 2: So https://github.com/openjdk/jmc/blob/master/application/org.openjdk.jmc.rjmx.ext/META-INF/services/javax.management.remote.JMXConnectorProvider is not detected by
Bottom line seems to me that the JMX protocol extension mechanism no longer works in JMC 9 (?) |
Hi! What you mentioned about how the Unfortunately it looks like I only considered the case of the rjmx.service extension, and not rjmx.ext. I think the Edit: I've opened a bug for this on the JMC jira: https://bugs.openjdk.org/browse/JMC-8178 |
@aptmac I believe the comments should be addressed now. |
Excellent. I'm off today but will take a look on Monday! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. The copyright script is probably just complaining now that upstream/master has new commits, I tried a rebase on-top of master and running the script and it came back clean. For completeness you could just rebase one last time to re-run that script, but should be good to go afterwards.
@aptmac : Merged in master and added jolokia to 2024-03. |
@aptmac : What is the process going forward here? I actually have a couple of follow up PR's where it would be good to get review started. How and when do PRs get merged? |
@skarsaune Not sure why the instructions got buried, but refer to this comment from the Skara bot: #548 (comment) Basically you type Once it's in feel free to rebase and open up new PRs, and if you need any help creating issues in JIRA let me know as well. |
/integrate |
@skarsaune |
/sponsor |
@aptmac @skarsaune Pushed as commit 8332466. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Replaces parts of #332
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jmc.git pull/548/head:pull/548
$ git checkout pull/548
Update a local copy of the PR:
$ git checkout pull/548
$ git pull https://git.openjdk.org/jmc.git pull/548/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 548
View PR using the GUI difftool:
$ git pr show -t 548
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jmc/pull/548.diff
Webrev
Link to Webrev Comment